Conversation
`NativeWatcher.startWatching` resolves as soon as `fs.watch` returns, but on macOS that can be before any events are delivered. libuv only signals another thread to (re)create the FSEvents stream, and the stream reports only changes made after it starts: https://github.com/libuv/libuv/blob/2b4b918d3381100854250c89d5159d4206daafb7/src/unix/fsevents.c#L362-L372 So a change made immediately after startup can be silently missed. On a slow host the gap is long enough to hit regularly - it's the remaining flake in the Native watcher integration tests, where the suite's first event never arrives. 14 of 297 macOS test jobs since #1974 hit it, across all three Node versions. This adds a `probe` option for watcher backends. `Watcher` passes one that writes a health check file to the backend's root and waits for it to be reported, and `NativeWatcher` probes until one is observed before resolving. If it can't confirm within 10s, Metro warns and carries on. Note that `NativeWatcher`s for multiple roots share one FSEvents stream, which restarts as each root is added, so the guarantee is for `Watcher.watch()` as a whole rather than each backend. Changelog: ``` - **[Fix]**: Fix changes made immediately after startup sometimes being missed on macOS ``` Test plan: New unit tests for the probe loop. The integration tests now pass their own probe. To reproduce the race locally, I patched `NativeWatcher` to drop all events for its first 300ms: - The Native integration tests fail as on CI without a probe ("detects a new, changed, deleted file" times out in its hook), and pass with this PR. - A `FileMap` in watch mode with two roots, writing a file to each as soon as `build()` resolves, sees neither change without a probe (3/3 runs) and both with this PR (3/3). - Dropping events indefinitely, `build()` resolves after 10s with a warning per root, and no health check files are left behind.
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
NativeWatcher.startWatchingresolves as soon asfs.watchreturns, but on macOS that can be before any events are delivered. libuv only signals another thread to (re)create the FSEvents stream, and the stream reports only changes made after it starts:https://github.com/libuv/libuv/blob/2b4b918d3381100854250c89d5159d4206daafb7/src/unix/fsevents.c#L362-L372
So a change made immediately after startup can be silently missed. On a slow host the gap is long enough to hit regularly - it's the remaining flake in the Native watcher integration tests, where the suite's first event never arrives. 14 of 297 macOS test jobs since #1974 hit it, across all three Node versions.
This adds a
probeoption for watcher backends.Watcherpasses one that writes a health check file to the backend's root and waits for it to be reported, andNativeWatcherprobes until one is observed before resolving. If it can't confirm within 10s, Metro warns and carries on.Note that
NativeWatchers for multiple roots share one FSEvents stream, which restarts as each root is added, so the guarantee is forWatcher.watch()as a whole rather than each backend.Changelog:
Test plan:
New unit tests for the probe loop. The integration tests now pass their own probe.
To reproduce the race locally, I patched
NativeWatcherto drop all events for its first 300ms:FileMapin watch mode with two roots, writing a file to each as soon asbuild()resolves, sees neither change without a probe (3/3 runs) and both with this PR (3/3).build()resolves after 10s with a warning per root, and no health check files are left behind.